Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #17465 +/- ##
============================================
+ Coverage 81.88% 95.87% +13.98%
============================================
Files 168 114 -54
Lines 9959 5280 -4679
============================================
- Hits 8155 5062 -3093
+ Misses 1658 218 -1440
+ Partials 146 0 -146
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| /// @notice Mapping from Safe address to its guard configuration | ||
| mapping(Safe => GuardConfig) public safeConfigs; |
There was a problem hiding this comment.
timelockSafeConfiguration, as per the specs. So that it can be merged with LivenessModule.
| /// @dev MUST never revert | ||
| /// @param _safe The Safe address to query | ||
| /// @return The timelock delay in seconds | ||
| function viewTimelockGuardConfiguration(Safe _safe) public view returns (GuardConfig memory) { |
There was a problem hiding this comment.
Not needed, we can just query timelockSafeConfiguration
There was a problem hiding this comment.
I think I had trouble getting solidity to auto generate the getter with just mapping(Safe => Config) public timelockGuardSafeConfig; which surprised me. I'll leave this open so that I can test that out again.
| /// @notice Returns the list of all scheduled but not cancelled transactions for a given safe | ||
| /// @dev MUST NOT revert - NOT IMPLEMENTED YET | ||
| /// @return List of pending transaction hashes | ||
| function checkPendingTransactions(address) external pure returns (bytes32[] memory) { |
There was a problem hiding this comment.
| function checkPendingTransactions(address) external pure returns (bytes32[] memory) { | |
| function getAllScheduledTransactions(address) external pure returns (bytes32[] memory) { |
Or we can have getScheduledTransaction and getScheduledTransactions
There was a problem hiding this comment.
OK, I will assume that the full interface for those two functions is:
getScheduledTransaction(Safe _safe, bytes32 _txHash) returns (ScheduledTransaction);
getScheduledTransactions(Safe _safe) returns (ScheduledTransaction[]);
| /// @dev MUST NOT revert - NOT IMPLEMENTED YET | ||
| /// @return List of pending transaction hashes | ||
| function checkPendingTransactions(address) external pure returns (bytes32[] memory) { | ||
| return new bytes32[](0); |
There was a problem hiding this comment.
I wonder how to actually code this, since we don't remove scheduled transactions from storage.
We might need to keep an additional enumerable set or dynamic array where we add scheduled transactions (duplicating scheduledTransactions) but where they are removed once executed or cancelled.
Or we have 3 separate enumerable sets and we move transactions between them:
scheduledTransactions
cancelledTransactions
executedTransactions
It all feels quite messy
There was a problem hiding this comment.
Agreed on a separate conversation that keeping the scheduled but not executed or cancelled transactions in a EnumerableSet that gets a duplicate of the relevant contents in scheduledTransactions is the safest and cleanest option if we need this function.
maurelian
force-pushed
the
jm/timelock-nonnested
branch
from
3afbab9 to
2791608
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
- Add configureTimelockGuard function to allow Safes to set timelock delays - Validate timelock delay between 1 second and 1 year - Check that guard is properly enabled on calling Safe using getStorageAt() - Store configuration per Safe with GuardConfigured event emission - Add comprehensive test suite covering all spec requirements - Implement IGuard interface for Safe compatibility 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add clearTimelockGuard function to allow Safes to clear timelock configuration - Validate that guard is disabled before allowing configuration clearing - Check that Safe was previously configured before clearing - Delete configuration data and emit GuardCleared event - Add comprehensive test suite covering all spec requirements - Add new errors: TimelockGuard_GuardNotConfigured, TimelockGuard_GuardStillEnabled 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add internal _getGuard() helper to centralize guard address retrieval - Update configureTimelockGuard and clearTimelockGuard to use helper - Reduces code duplication and improves maintainability 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add cancellationThreshold function to return current threshold for a Safe - Return 0 if guard not enabled or not configured - Initialize to 1 when Safe configures guard - Clear threshold when Safe clears guard configuration - Add comprehensive test suite covering all spec requirements - Function never reverts as per spec requirements 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…lity - Add scheduleTransaction placeholder (Function 4) - Add checkPendingTransactions placeholder (Function 6) - Add rejectTransaction placeholder (Function 7) - Add rejectTransactionWithSignature placeholder (Function 8) - Add cancelTransaction placeholder (Function 9) - Update checkTransaction placeholder (Function 5) - All placeholders have proper signatures and documentation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Add CancellationThresholdUpdated event and emit it from threshold modification functions. Remove threshold parameter from TransactionCancelled event for cleaner separation of concerns.
We can simply use `timelock > 0` as an indicator of configuration
The right way to do this is now just to set timelockDelay to zero.
This library significantly reduces the amount of boilerplace required to setup a Safe transaction, then schedule, cancel, or execute it.
maurelian
force-pushed
the
jm/timelock-nonnested
branch
from
8f252c5 to
581cab0
Compare
| // TODO: this test fails because the guard config cannot be modified while the guard is | ||
| // disabled. IMO a guard should be able to manage its own configuration while it is disabled. | ||
| vm.skip(true); |
There was a problem hiding this comment.
Discussion on specs: ethereum-optimism/specs#761 (comment)
Avoids duplicating logic between specs and implementation
| uint256 blockingThreshold = _blockingThreshold(_safe); | ||
| uint256 quorum = _safe.getThreshold(); | ||
| // Return the minimum of the blocking threshold and the quorum | ||
| return (blockingThreshold < quorum ? blockingThreshold : quorum) - 1; |
There was a problem hiding this comment.
| return (blockingThreshold < quorum ? blockingThreshold : quorum) - 1; | |
| return (blockingThreshold < quorum ? blockingThreshold : quorum); |
The max_cancellation_threshold is min(blocking_threshold, quorum)
3 participants
Description
Adds a TimelockGuard contract, which is a Safe Guard to enable schedule and cancellation of transactions.
For a full spec, see ethereum-optimism/specs#761.